Skip to content

Conversation

sbendary25
Copy link

@sbendary25 sbendary25 commented Dec 3, 2023

Describe the changes you have made:

yaml.safe_load returns None from the get_config function if the config file is empty. This change checks for None value and instead returns {} so extend_config call doesn't throw exception when config file is empty.

Reference any relevant issue (Fixes #000)

#794

  • I have performed a self-review of my code:

I have tested the code on the following OS:

  • Windows
  • MacOS
  • Linux

AI Language Model (if applicable)

  • GPT4
  • GPT3
  • Llama 7B
  • Llama 13B
  • Llama 34B
  • Huggingface model (Please specify which one)

@Notnaton Notnaton linked an issue Dec 4, 2023 that may be closed by this pull request
Copy link
Contributor

@CyanideByte CyanideByte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the code @sbendary25 provided on Windows and it works great. I think you could go a step further and have it repair the config file using the default one. Here is some tested code below that does that.

def get_config(path=user_config_path):
    path = get_config_path(path)
    
    config = None

    with open(path, "r") as file:
        config = yaml.safe_load(file)
        if not config is None:
            return config

    if config is None:
        # Deleting empty file because get_config_path copies the default if file is missing
        os.remove(path)
        path = get_config_path(path)
        with open(path, "r") as file:
            config = yaml.safe_load(file)
            return config

@sbendary25
Copy link
Author

added @CyanideByte suggestion a90cc8f

@sbendary25
Copy link
Author

@Notnaton also added a message to inform user that default config is being applied

@sbendary25
Copy link
Author

@Notnaton bumping this to be closed and merged.

@KillianLucas
Copy link
Collaborator

Great work @sbendary25. And thanks @CyanideByte and @Notnaton for your suggestions and reviews. Merging now.

(also thanks for your comments on the latest --os updates Samer— you have a great attention to detail)

@KillianLucas KillianLucas merged commit 6018285 into openinterpreter:main Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

If config is empty interpreter crashes

4 participants